[sisyphus-bot]

Early-stage review — heads up for when this is ready

This is looking like a solid approach. The two-commit structure is clean: first commit replaces fixCorners with the 'rounded' curve and generateRoundedPath, second commit fixes the curve resolution so it works across all diagram types. Some items to consider before marking ready for review.

What's working well

🎉 [praise] The resolveEdgeCurveType() helper (edges.js:34-43) is elegant. A single exported function, well-documented, tested in isolation — it solves the "class diagrams don't get rounded edges" problem at exactly the right level in the pipeline instead of requiring changes in every diagram DB.

🎉 [praise] Removing fixCorners (86 lines of manual geometry with magic numbers like Math.sqrt(2) * 2) in favor of generateRoundedPath (which was already in the file) is a clean improvement. The new function handles edge cases better (collinear points, marker offsets) and is easier to reason about.

Things to address

🟡 [important] Breaking change not flagged. The config default changes from 'basis' to 'rounded' (config.schema.yaml:2158-2159). Per CLAUDE.md, "changing default rendering output in ways visible to consumers" constitutes a breaking change. Every flowchart that doesn't explicitly set curve: will switch from smooth splines to right-angle-with-rounded-corners. This is an intentional design improvement, but it should be:

• Called out in the PR description as a breaking/visual change
• Reflected in the changeset semver (at least minor, arguably major)
• Noted for the release notes so consumers aren't surprised

🟡 [important] E2E coverage gap for non-flowchart diagrams. The fixCorners removal + default change affects ALL diagram types using the rendering-util edge path — class diagrams, state diagrams, etc. The e2e tests added only cover flowcharts (ELK and v2). Since rendering-util/ is shared code, the CLAUDE.md convention is: "Always run full e2e tests after modifying shared rendering code — unit tests alone are insufficient." Worth adding at least one e2e test for a non-flowchart diagram (class or state with ELK) to confirm the resolveEdgeCurveType fallback works visually.

🟡 [important] Missing changeset. No .changeset/*.md file in the diff. This is a user-facing rendering change that needs pnpm changeset — patch bump at minimum (since it's a bug fix), but given the default change, consider minor.

💡 [suggestion] Dead code on lines 510-511. let curve = curveBasis; curve = curveLinear; — the first assignment is immediately overwritten. This is pre-existing, but since you're already touching these lines, could clean it up to just let curve = curveLinear;.

💡 [suggestion] Config namespace consideration. resolveEdgeCurveType falls back to getConfig()?.flowchart?.curve — this means all diagram types (class, state, etc.) inherit the flowchart curve setting. This works pragmatically today since flowchart.curve is the only curve config. If this becomes a pattern, a top-level curve config key might be cleaner long-term. Not blocking — just noting the architectural direction.

Summary

Verdict: COMMENT (draft PR — all items are heads-up, not demands). The core approach is right. The main gap is confirming the visual impact across diagram types with e2e tests and adding a changeset before this moves out of draft.

[sisyphus-bot]

Nice work on this — the approach is clean and directly addresses #7213. A few heads-ups for when you're ready to finalize.

What's working well

🎉 [praise] The removal of fixCorners, extractCornerPoints, and findAdjacentPoint (~90 lines) in favor of generateRoundedPath is a great cleanup. The quadratic Bezier approach is more mathematically sound and handles edge cases (zero-length segments, near-collinear points) gracefully. Much nicer than the old manual offset logic.

🎉 [praise] The resolveEdgeCurveType helper is a smart abstraction — it cleanly separates the "what curve type are we using?" question from the D3 curve factory selection. The unit tests covering string, undefined, null, and function inputs give good confidence in the resolution logic.

🎉 [praise] Test coverage is solid: the ELK e2e test reproduces the exact scenario from #7213, the flowchart-v2 test exercises the new rounded curve option explicitly, and the unit tests verify the config fallback logic. This is the right combination of unit + visual regression tests for a rendering-util change.

Things to consider

🟡 [important] — Default change is a visual breaking change. Changing the default flowchart.curve from 'basis' to 'rounded' will alter every existing flowchart rendering across GitHub, GitLab, Obsidian, and all other consumers. The project's own guidelines list "changing default rendering output in ways visible to consumers (edge paths, spacing)" as potentially breaking. The changeset uses a minor bump — worth considering whether this should be major, or at minimum confirming this is an intentional trade-off you're comfortable with. The migration note in the changeset ("set flowchart.curve: 'basis'") is great — just want to flag the semver question. (config.schema.yaml:2160)

🟡 [important] — Cross-diagram regression risk for non-flowchart types. When edge.curve is a D3 CurveFactory function (as class diagrams, state diagrams, and others may pass), the old code would fall through the switch to default: curveBasis. The new resolveEdgeCurveType resolves those to the flowchart config default ('rounded'), which then uses curveLinear + generateRoundedPath instead of curveBasis. This could change edge rendering for non-flowchart diagram types unintentionally. Worth a quick visual check on a class diagram and state diagram to confirm they still look right, or adding a guard so non-string edge.curve values that are actual D3 functions get applied directly rather than falling through to config. (edges.js:41-42)